-
Notifications
You must be signed in to change notification settings - Fork 351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ref #3023: Add native compilation with sources #4021
Conversation
BTW, if you want to run the native test suite as well, you need to call the native workflow directly on the branch as we have moved them into nightly releases (they used to eat up a lot of memory):
|
1870e16
to
ea59a8d
Compare
As I'm working on some ideas around Camel K 2.0 (gitops, being able to build any Camel application, ...), let me share with you something I've been thinking lately. I may probably create an issue with some visual support, but hopefully can help you in the work you're doing. I think that instead of passing the whole content of a source along the different CRs, we should have the concept of a "generated Maven project" (it may even come from Once the Project is |
Details of the proposal defined in the previous comment exposed in #4024 |
Your idea is great however it is scoped to Camel K 2.0 while I would expect a solution for Camel K 1.17, don't you believe that it could be an acceptable approach for 1.17? |
d361b70
to
e11efff
Compare
@@ -216,6 +265,9 @@ func (t *quarkusTrait) newIntegrationKit(e *Environment, packageType traitv1.Qua | |||
Traits: propagateKitTraits(e), | |||
} | |||
|
|||
if packageType == traitv1.NativePackageType { | |||
kit.Spec.Sources = propagateSourcesRequiredAtBuildTime(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 good to include only when native
I guess you mean |
e11efff
to
10706ec
Compare
@squakez your remarks have been addressed.
|
Cool, I still need to have a deeper look, but in the while, about the native test I suggest you move them into a separate workflow (similar of what we used to have before) so that they won't timeout because the rest of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good for me. Let's see how the checks perform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
2e7c1aa
to
8f5d4fa
Compare
8f5d4fa
to
89579af
Compare
Infortunatelly the native E2E test cannot pass on the GitHub CI because it requires more than 6 Go of RAM, so I need to remove it. |
89579af
to
5716960
Compare
Have you rebased it? it seems there was some failure I've fixed in #4022 - we need to find a way to run the native check smoothly anyhow, maybe we can run it as an additional nightly task right after the release task (to avoid resource consumption conflict). |
0ea14eb
to
eca430c
Compare
c5e02f2
to
aaf89ad
Compare
c4b6d90
to
b462918
Compare
Sounds good to be reviewed now, the build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good but a little change to avoid affecting the rest of testing. Great work!
@@ -23,11 +23,12 @@ runs: | |||
steps: | |||
- id: install-cluster | |||
name: Install Cluster | |||
uses: container-tools/kind-action@v1 | |||
uses: container-tools/kind-action@61f1afd4807b0dac84f3232ec99e45c63701d220 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can merge with this commit id, but, please, create a follow up issue to release the kind-action and update here accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it is #4063
env: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
|
||
on: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we have verified for this release, let's change this to run only on a nightly schedule to avoid such a high resource consumption process for every PR or merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, as you like but I did my best to avoid increasing the overall duration of a full build as explained in this comment #4021 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and it's a great effort indeed. However, native workflow is too much resource consuming and we don't have enough capacity to run at every github action. We still have a daily view of what's going on, though.
cancel-in-progress: true | ||
|
||
jobs: | ||
install-native-high-memory: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 2 different jobs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, it was to be able to launch the "lightest" tests on a Linux runner but they still need more than what a Linux runner can offer. Now, it is still interesting as they are launched in parallel so it allows having a duration that is equivalent to the duration of common / common-it
which also means that the overall duration of a full build is not really affected.
|
||
// TestTimeoutVeryLong should be used only for testing native builds. | ||
var TestTimeoutVeryLong = 90 * time.Minute | ||
var TestTimeoutVeryLong = 60 * time.Minute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We better provide a specific timeout for the native test, no needs to use global variables. With this change we are slowing down also any other test that would expect those previous values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, actually it is the opposite, I reduced the duration of the timeout by 30 minutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was more concerned about TestTimeoutLong
which is increased a little bit. I wonder if we could instead introduce a different timeout variables for native (ie, NativeTestTimeoutLong
) or either just use the quantity of minutes. But we refine this in a following iteration, not a blocker for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I had to change a bit the timeout because, on MacOS runners, everything is much slower probably because it needs a container runtime which is not the case with Linux. However in the end, If a test works as expected the timeout set should not have any effect on the duration of the test, it is only when something goes wrong the state is not the expected one.
b462918
to
839aceb
Compare
Good for me. As it's all green let's merge this :) - we will understand if moving the |
|
||
// TestTimeoutVeryLong should be used only for testing native builds. | ||
var TestTimeoutVeryLong = 90 * time.Minute | ||
var TestTimeoutVeryLong = 60 * time.Minute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was more concerned about TestTimeoutLong
which is increased a little bit. I wonder if we could instead introduce a different timeout variables for native (ie, NativeTestTimeoutLong
) or either just use the quantity of minutes. But we refine this in a following iteration, not a blocker for this PR.
Excellent work. Great feature to promote for 1.12.0 release! |
fixes #3023
Motivation
Some DSLs need to have the sources present at compile time in native mode to be able to generate classes and include them in the native image.
Modifications
maven/src/main/resources/routes
Release Note